Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main: build binary distributions via sdists by default #304

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Jun 7, 2021

Fixes #257

Signed-off-by: Filipe Laíns [email protected]

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +0.5 on this, but this would like a way to force the old behavior from the cli (build a wheel from the source directly). We've basically doubled the build time for a wheel, which sometimes can be a big regression.

src/build/__main__.py Outdated Show resolved Hide resolved
@FFY00
Copy link
Member Author

FFY00 commented Jun 7, 2021

This patch only affects invoking python -m build without --sdist/-s or --wheel/-w. If you don't specify any of them, we will build sdist and then a wheel from it, if you specify any, you will get the previous behavior. python -m build -w will keep building the wheel directly.

python -m build - build a sdist and then a wheel from the sdist (modified behavior)
python -m build -s - build a sdist (old behavior)
python -m build -w - build a wheel directly (old behavior)
python -m build -s -w - build a sdist and wheel directlt (old behavior)

Essentially, if no distribution is specified, we will build a wheel from sdist, if any are, we will just build those distributions directly.

I forgot to touch up the help messages, I should fix that.

@henryiii
Copy link
Contributor

henryiii commented Jun 7, 2021

I'm +0.5 on this as well. Initially it seemed like it could be problematic, but every argument I could come up with against it already has an answer or obvious workaround (python -m build --sdist --wheel if you really need the old behavior, packages that can't build sdists won't build sdists anyway, etc). It could help catch bugs (SDist unable to build wheel), though it might not always "catch" them and could distribute wheels with missing files, but you would already be distributing broken SDists in that case. The biggest issue remaining is making it clear in the documentation that this is how it works - as long as it is clear that "default" is to do source -> sdist -> wheel instead of being a shortcut for "--sdist --wheel", I think it's fine.

src/build/__main__.py Outdated Show resolved Hide resolved
src/build/__main__.py Outdated Show resolved Hide resolved
src/build/__main__.py Outdated Show resolved Hide resolved
@layday
Copy link
Member

layday commented Jun 8, 2021

I sympathise with not wanting to add more options to the CLI, but I don't like that has different semantics from -sw all of a sudden. If the default is to build an sdist and a wheel, why would anyone expect that -sw does something different? I don't think the documentation can compensate for the shortcoming in usability. Can we think of something else we can do here, even if it's simply to disallow -sw?

@webknjaz
Copy link
Member

webknjaz commented Jun 8, 2021

@FFY00 what about python -m build --wheel dist/some-tarball.tar.gz? I don't see this case solved, am I missing anything? This is an important use case specifically for building platform-specific matrixes of wheels.

@FFY00
Copy link
Member Author

FFY00 commented Jun 15, 2021

That is a different change, this PR is only for the default behavior. I know you mentioned this in #257, but can you open a separate issue for that? So that we can track it as an actionable task.

@FFY00 FFY00 force-pushed the build-via-sdist branch 2 times, most recently from e794906 to 57d9c81 Compare June 15, 2021 18:44
@FFY00
Copy link
Member Author

FFY00 commented Jun 15, 2021

The last commit will also allow us to move the colorama stuff and warnings.showwarning overriding to main in a next PR.

@FFY00
Copy link
Member Author

FFY00 commented Jun 15, 2021

If the default is to build an sdist and a wheel, why would anyone expect that -sw does something different? I don't think the documentation can compensate for the shortcoming in usability.

The people that would be affected by that are advanced users, which should not really have much of an issue reading the documentation. I am optimizing for the normal user, which would be the one that would fall into that documentation issue. Almost all normal users should actually want to build binary distributions via the source distribution.
I acknowledge this is not an optimal solution, but so far I don't think such exists. We already discussed this in #257 and so far no better solution that matches these requirements was presented.

src/build/__main__.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

That is a different change, this PR is only for the default behavior. I know you mentioned this in #257, but can you open a separate issue for that? So that we can track it as an actionable task.

Right. Makes sense. Done: #311.

@fuglede
Copy link

fuglede commented Jun 21, 2021

This is probably obvious but caused me some gray hairs, so let spell it out here:

After a bit of digging, this change in particular appears to be the cause of my builds breaking, by imposing new assumptions on folder structure: I have a project in which a Cython pxd includes C++ code through

cdef extern from "../my/relative/path/file.cpp"

This works fine with python -m build (and -s/-sw) with build 0.3.1.post1, but with 0.5.0, only python -m build -w and -sw work (for the empty argument list, or -sw, compilers (MSVC/clang++) will complain that file.cpp cannot be found).

@FFY00
Copy link
Member Author

FFY00 commented Jun 21, 2021

That is exactly the kind of issue this change is supposed to be uncovering! It means your package cannot be built from the sdist. If there is no compatible wheel on PyPI, pip will try to build the package from the sdist.

You have two options:

  1. Ship the code in the sdist, so that it can be used to build the package
  2. Acknowledge that the package cannot be built from a sdist, which means the only supported platforms are the ones you are shipping wheels for. In this case, you should not be uploading a sdist to PyPI, as it is designed to be broken.

@gaborbernat
Copy link
Contributor

I think that exposes a valid bug though. Surely a sdist created like that would also fail, because that relative path is not exist at the users machine either 🤔

@henryiii
Copy link
Contributor

I'm curious, if you have compiled code, are you only building one wheel (one OS/arch/Python version), or are you building identical sdists every time? Normally, I'd expect one SDist job and multiple wheel jobs (such as in https://scikit-hep.org/developer/gha_wheels). Build #311 (and cibuildwheel pypa/cibuildwheel#597) do not support running directly from the SDist, though I think it would be a good idea to be able to do that, to encourage this structure even in that case. :)

@fuglede
Copy link

fuglede commented Jun 22, 2021

Acknowledge that the package cannot be built from a sdist, which means the only supported platforms are the ones you are shipping wheels for. In this case, you should not be uploading a sdist to PyPI, as it is designed to be broken.

FWIW, this was exactly my use case; an sdist-less internal package designed for use in controlled environments for which the source code could not meaningfully be bundled, and for which only wheels were necessary.

@gaborbernat
Copy link
Contributor

So you want to only build wheel then via python -m build -w, that should still build the wheel via the source tree.

@henryiii
Copy link
Contributor

There's absolutely nothing wrong with building only wheels; there is something deeply and inherently wrong about building SDists that are lies, being known to be invalid and broken. ;) So, yes, just ask for wheel-only and everything will be fine. The default SDist + wheel is the only thing that has changed (to SDist -> wheel). python -m build --wheel is what you actually wanted.

@henryiii
Copy link
Contributor

Can I change my vote after-the-fact to a strong +1? This is finding bugs even in build backends! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build wheel from sdist (optionally?)
6 participants